Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1단계 - 영화 목록 불러오기] 윤생(이윤성) 미션 제출합니다. #6

Merged
merged 52 commits into from
Mar 25, 2023

Conversation

2yunseong
Copy link

@2yunseong 2yunseong commented Mar 16, 2023

안녕하세요 티케! 윤생입니다!

봄이 오고 있는지 꽃샘추위로 날씨가 많이 춥네요! 감기 조심하세요!!

레벨 1 마지막 미션 이네요! 2주간 잘 부탁드립니다 :D

컴포넌트로 분리한 이유 - 재사용성

  • 요구사항을 작성하고 분석해보니 재사용 되는 컴포넌트가(영화 아이템, 더보기 아이콘) 생각보다 많겠다는 생각이 들었습니다.
  • 그래서 앱의 기능이 다양해져도 이런 재사용 가능한 컴포넌트가 많이 사용될 것 같다는 생각이 들었습니다.
  • 따라서, 이번 미션에서 컴포넌트의 관점은 어떻게 재사용이 가능한 UI 컴포넌트로 분리할 수 있는지를 고민을 하게 되었어요.

웹 컴포넌트를 사용한 이유?

  • 요구사항을 정리해 분석한 결과 우리 앱에 가장 필요한 재사용성을 늘릴 수 있다고 생각했습니다. 이 부분을 웹 컴포넌트를(커스텀 컴포넌트) 선택한 가장 핵심적인 이유였습니다.

개발자로서 우리 모두는 가능한 한 코드를 재사용하는 것이 좋은 생각이라는 것을 알고 있습니다. 이는 전통적으로 커스텀 마크업 구조에선 쉽지 않았습니다. 커스텀 UI 컨트롤을 렌더링하기위해 작성해야하는 복잡한 HTML (및 관련된 스타일과 스크립트)을 생각해보십시오. 여러번 사용할 때 조심하지 않으면 페이지가 엉망이 될 수 있습니다.

웹 컴포넌트는 다음 문제들을 해결하는 것을 목표로 합니다 — 세 가지 주요 기술들로 구성되며, 재사용을 원하는 어느곳이든 코드 충돌에 대한 걱정이 없는 캡슐화된 기능을 갖춘 다용도의 커스텀 엘리먼트를 생성하기 위해 함께 사용될 수 있습니다.

-mdn -

이벤트 위임을 사용한 이유?

  • 도메인 관련 로직을 최대한 상위로 빼, 재사용이 가능한 컴포넌트들에게 로직의 책임을 덜 관여하게 하고 싶었음을 의도하고 싶었습니다.
  • 오로지 사용자에게 데이터에 따라 화면을 보여주는 역할만 부여해, UI 컴포넌트의 재사용성을 더하고 싶었어요.

구현 방향

위 내용과 더불어, 다음과 같이 생각해보았는데요,

  • 가급적 작은 단위의 컴포넌트에 도메인 로직에 대한 책임을 분리하려 했습니다. 따라서 컴포넌트는 자신이 받은 데이터에 따라 화면에 어떻게 보여지는 지에 대한 책임만을 가진다고 생각하였습니다.
  • 최상위 컴포넌트에 등록된 이벤트 핸들러는 전역 상태를 가지면서 도메인 로직을 관장한다고 생각하였습니다.

사용자 측면 고민

어떻게 보면 사용자로 몇십년을 살아왔는데, 아직도 개발할 때 사용자의 입장에서 생각하는게 어려운 것 같습니다. 하지만, 다음과 같은 부분을 고민해보았어요. 티케의 생각도 자유롭게 들려주시면 좋을 것 같아요.

  • 검색 결과가 일치 하지 않을 때, 사용자에게 검색어를 개선 제안을 해보았습니다.
  • 마지막 페이지 일때는 의도적으로 더보기 버튼을 숨겼어요.
  • 한번 로드된 데이터는 브라우저 자체의 캐싱으로 인해 제가 의도치 않게 성능이 개선되더라구요.. 처음 알았습니다!
  • 요구사항에는 없었지만 로고를 눌렀을 때 인기항목 첫번째 페이지를 로드 해오도록 했습니다. 로고를 누르는 동작 자체가 사용자에게 익숙한 방법이라 추가해야 겠다고 생각했습니다 ..!
  • 검색어가 없이 엔터를 치거나 검색버튼을 누르면 경고창이 나와요.
  • 인기 영화 리스트가 20개씩 DOM에 페이지처럼 계속 추가되는 구조로 구현했습니다. api를 보니 최대 페이지의 크기가 1000페이지 정도 되는데, 영화 목록 자체를 상태로 관리한다는 건 맞지 않다고 생각이 들었어요. (목록의 상태가 바뀔 때 마다 새로 렌더링 한다면…)

다음은 생각은 했지만.. 제한 시간 때문에 우선순위에서 밀려 아직 구현하지 못한 부분이에요.

  • 더보기를 계속 누르다 보면 헤더를 찾을 수 없이 한없이 밑으로 내려가 버려요. 그래서 사용자에게 헤더를 뷰포트에 고정시키거나 위로가기 버튼을 제공해야 될 것 같아요.
  • 가끔 api 문제 때문에 img url 이 잘 안불러와져, 사용자에게 alt텍스트로 보이는 부분이 있는데, 대체 이미지로 보여주면 좋을 것 같다는 생각입니다.
  • 사용자에게 리스트뷰/이미지뷰 를 셀렉트바 로 선택할 수 있게 제공해도 좋을 것 같아요. 가끔은 텍스트로만 보고 싶은 사용자도 있을 수 있으니까요.
  • 페이지 스크롤 끝에서 아래 화살표를 누르면 더보기 버튼이 눌러지는 역할을 해도 직관적일 거란 생각을 했어요.
  • 요구사항은 검색버튼 클릭/엔터 를 쳐야 검색이 되었지만 사용자가 입력하는 즉각 검색이 되어도 좋겠다는 생각을 해봤습니다.

질문

  • 사용자에게 상세히 에러를 보여 줄 필요가 없다고 생각이 들어서 오류 페이지를 통일 시켰는데, 티케의 생각은 어떠신지 궁금합니다!
  • e2e test로 cypress를 사용하였는데, 단언(should) 없이 contains로만 작성해도 어색하지 않는지, 아니면 반드시 should를 써줘야 하는지 궁금합니다!
    • 왜냐하면 contains의 결과 자체가 무언가를 포함하고 있는지 암시적으로 판단한다고 생각하여, 따로 적어주지는 않았습니다. 따라서 티케의 생각이 궁금해요!

사용한 API

홈페이지 : TMDB

인기 영화 목록

자세한 링크 : GET /movie/popular

최신(?) 인기 영화를 불러옵니다.

요약

(앱에서 사용하는 정보 이외의 요청/응답 값은 생략 하였습니다. 자세한 내용은 위 링크 참고 바랍니다.)

요청 (QueryString)

query type require description
api_key string required api 키
language string option 언어옵션. 이번 미션에서는 ko-KR로 고정
page integer option 결과 페이지

응답 (성공 시)

property type description
page integer 현재 페이지
results array[object] 결과(영화 정보) 객체가 담긴 배열
total_results integer 전체 영화 갯수
total_pages integer 전체 페이지

results 상세 값

property type description
poster_path string or null 영화 포스터 url
title string language 값에 따른 제목
original_title string 영어 제목
vote_average number 평점

영화 검색

링크 : GET /search/movie

영화를 검색합니다.

요약

(앱에서 사용하는 정보 이외의 요청/응답 값은 생략 하였습니다. 자세한 내용은 위 링크 참고 바랍니다.)

요청 (QueryString)

query type require description
api_key string required api key
language string option 언어옵션. 이번 미션에서는 ko-KR로 고정
query string option 검색어
page integer option 결과 페이지

응답 (성공 시)

property type description
page integer 현재 검색된 페이지
results array[object] 결과(영화 정보) 객체가 담긴 배열(최대 20개)
total_results integer 전체 영화 갯수
total_pages integer 전체 페이지

results 상세 값

property type description
poster_path string or null 영화 포스터 url
title string language 값에 따른 제목
original_title string 영어 제목
vote_average number 평점

API 키 사용방법

  • README.md에 정리해두었습니다. API 키는 별도로 드리도록 하겠습니다.

Other

배포 링크

구조 설계도
component 구조

ksone02 and others added 30 commits March 14, 2023 17:13
Co-authored-by: 2yunseong <2yunseong@users.noreply.github.com>
Co-authored-by: 2yunseong <2yunseong@users.noreply.github.com>
@devhyun637 devhyun637 self-assigned this Mar 16, 2023
}

searchListInit() {
this.#nextPage = 1;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 값에 직접적으로 접근하는 로직은 하나하나 메서드로 추상화 해줘야 하는지 고민이 됩니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 이 질문이 잘 이해가 안가는데, 자세한 설명을 부탁드려도 될까요~?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

searchListInit() 은 사용자가 검색을 하면 인기 영화 목록에서 검색 결과 목록의 화면으로 바뀌게 됩니다.

이 때, 검색 결과를 가져오기 위해 현재 페이지가 몇 페이지인지 알고 있어야 되는데요, 초기 검색은 검색 결과의 첫번째 페이지를 요청해야 하니 위 코드 처럼 page 상태를 갱신 시켜줍니다.

여기서 궁금했던 부분은 searchListInit() 에서는 page의 상태에 (nextPage 변수에) 직접 변경을 가하게 되는데, 이런 작업은 추상화 해줘야 되는지 궁금했습니다!

지금 보면 1이라는 숫자가 매직넘버 같기고 하고, 메서드를 사용하지 않고 변수에 직접 접근해서 조작을 가하는게 코드 내에서도 무슨 의도인지 빠르게 알 수 없을 것 같다는 생각이 들면서도, 한줄 짜리 간단한 코드도 굳이 추상화 해줘야 할 만큼 의미가 있을까 라는 두 자아가 싸우고 있어 질문 드렸습니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

searchListInit()에서 첫 페이지로 맞춰줄 때, 1이라는 숫자를 사용하는 것에 대한 고민이 맞을까요?
저는 그렇다면 이정도는 충분히 추상화를 하지 않고 1을 명시해주어도 좋다는 의견을 드리고 싶습니다!
init이라는 함수명에서도 초기화라는 의미를 담았고, 1이라는 숫자는 첫 페이지라고 하기에 명확하니 매직넘버처럼 느껴지지는 않아요 :)

@woowahan-cron
Copy link
Contributor

PR 본문이 이렇게 상세할 줄이야..
내일 수업 시간에 PR 쓰는 법에 관해서 공지 나갈 건데
윤생에게는 잔소리가 될 것 같아서 미리 양해를 구합니다 🙏

@2yunseong
Copy link
Author

@woowahan-cron 오히려 너무 장황하지 않나 고민하던 참이였습니다..!! ㅎㅎ 내일 수업 기대되네요!!

Copy link

@devhyun637 devhyun637 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 윤생~ 일찍 미션을 제출해줬는데 리뷰가 늦어서 죄송해요ㅠㅠ 😢😢
이번에 비동기를 제대로 다루는 미션이었는데 어떠하셨나요?
코드가 깔끔해서 읽는데 어려움이 크게 없었고, MR이 자세해서 어떤 의도로 코드를 짰는지 알 수 있어서 좋았습니다~

의견

사용자에게 상세히 에러를 보여 줄 필요가 없다고 생각이 들어서 오류 페이지를 통일 시켰는데, 티케의 생각은 어떠신지 궁금합니다!

🔖 저는 사용자에게 친절하면 할수록 좋다는 의견을 드리고 싶네요. 물론 개발자가 보는 에러를 바로 보여주는 것은 친절하지 않으니, 오류 상황일 때 풀어서 설명해주면 좋겠죠? '알 수 없는 오류'라고 하면 '그래서 어쩌라고~ 너 안써!' 이렇게 될 수 있으니깐요.. 그리고 오류 페이지에도 CTA 버튼이나, 사용자가 할 수 있는 Action을 안내해주면 어떨까 하는 생각이 듭니다.

최상위 컴포넌트에 등록된 이벤트 핸들러는 전역 상태를 가지면서 도메인 로직을 관장한다고 생각

🔖 지금은 작은 프로젝트라서 최상위 컴포넌트가 모든걸 다 가지고 이벤트를 핸들링 하는게 가능하겠지만, 추후에 조금만 프로젝트가 복잡해지면 '전역에서 사용하는 것은' 별로 좋은 방법은 아니라고 생각해요 :) 또 최상위 컴포넌트가 하위 컴포넌트가 하는 일을 다 알아야하나?도 고려해보면 좋을 것 같아요. 하지만 이벤트 위임을 사용하는 이유에 대해서 본인만의 생각이 있고 시도해본건 좋습니다 👏👏

🔖 제가 제안할만한 UX적인 부분들을 아직 구현을 못했다고 남겨주셨군요! :) 제 생각도 거의 동일해서 따로 코멘트는 남기지 않겠습니다~ 피드백을 반영하면서 같이 구현해주면 좋을 것 같아요. 아! 한가지. href#로 되어있어서 cursor를 올렸을 때 클릭할 수 있을 것 같은데 클릭해도 아무런 반응이 일어나지 않아서 이 부분이 아쉽네요! 없는 기능이라면 사용자를 헷갈리지 않게 하는게 좋겠죠?

🔖 질문) 검색 결과가 일치 하지 않을 때, 사용자에게 검색어를 개선 제안을 해보았습니다.
-> 이 부분 궁금한데 어떻게 해야 제가 확인할 수 있을까요?

Comment on lines +2 to +5
beforeEach(() => {
cy.visit("http://localhost:8081/");
cy.viewport(1920, 1080);
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
적용안된 부분이 보이는데 적용해주시면 좋을 것 같아요~

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

viewport 말씀하시는건가요?? intercept를 접속 이전에 실행해야 되는걸로 알고 있어 나머지는 중복으로 작성했는데, 다시 한번 알아볼께요!

cypress/e2e/movie-e2e.cy.ts Outdated Show resolved Hide resolved
cypress/e2e/movie-e2e.cy.ts Outdated Show resolved Hide resolved
cypress/support/commands.ts Outdated Show resolved Hide resolved
cypress/support/e2e.ts Outdated Show resolved Hide resolved
src/components/movie/MovieComponent.js Outdated Show resolved Hide resolved
this.#page.setAttribute("data-status", "no-result");
return;
}
this.#page.setAttribute("data-movie-list", JSON.stringify(movieItems));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

html에 너무 많은 정보가 담긴건 아닐까요?
스크린샷 2023-03-18 오후 9 45 11

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

구현 할 때는 dataset을 통해 html 태그에 직접 데이터를 바인딩해주면 편리하지 않을까 생각해 dataset을 사용하였습니다! 또한, 웹 컴포넌트에서 속성이 변경되었을 때 실행되는 콜백을 등록할 수 있어 유용하다고 판단해 사용하였습니다.

너무 많은 정보가 담기는 것 이외에 또 어떤 단점들이 있을까요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

데이터가 많이 담기게 되면, 페이지 로딩 속도가 저하될 수 있으므로 성능저하의 문제가 있을 것 같아요.
또, 만약에 영화정보가 아닌 사용자의 정보였다면 중요한 정보가 노출될 수 있다는 문제가 있을 수 있겠죠?

다른 문제들은 저도 아직 잘 모르겠지만, 일단 다른 개발자들이 개발자도구를 활용해서 이 페이지를 추가적으로 유지보수 할 때 가독성이 저하될 수 있겠다는 건 저도 경험한 것 같네요 :/

src/components/movie/MovieListComponent.js Show resolved Hide resolved
src/index.js Show resolved Hide resolved
src/util/MovieList.ts Outdated Show resolved Hide resolved
Copy link
Author

@2yunseong 2yunseong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 티케~ 날씨가 조금 따뜻해지려 하더니 미세먼지가 왕창 찾아오네요 🥲

각설하고 이번 미션을 하면서 비동기에 대해 깊게 파본 것 같아요!! 재미있었습니다 :D

커멘트에 남겨주신 내용과 더불어 몇 가지 반영한 내용으로 다시 리뷰 요청드립니다~!! 또 질문 주신 내용에 대해 몇가지 커멘트를 남겼습니다~!

특히 dataset을 이용한 상태관리는 많은 리팩터링이 필요할 것 같아 일단은 커멘트만 남겨두었으니 다른 피드백 받은 것 부터 적용해보았어요. 차차 적용하면서 작은 단위로 자주자주 요청 보내보도록 노력하겠습니다!

++ 추가로 fetch 관련해서 의견주신 것도 다양한 상황에서 사용해볼 수 있도록 스스로 고민을 좀 더 해보고 리팩터링 해보겠습니다!

🔖 질문) 검색 결과가 일치 하지 않을 때, 사용자에게 검색어를 개선 제안을 해보았습니다.
이 부분 궁금한데 어떻게 해야 제가 확인할 수 있을까요?

이 부분은 검색 실패 시라고 말씀드렸어야 하는데, 제가 어휘 선택을 잘못했네요. 아래 사진처럼 검색 실패 결과를 의미하였습니다.
Screenshot 2023-03-20 at 23 13 05

결과가 없으면, 사용자에게 검색어를 바꿔보는 걸 제안하도록 구현 해보았습니다~!

(글쏨씨가 부족한 점 양해부탁드립니다 😥)

감사합니다 :)

src/util/MovieList.ts Outdated Show resolved Hide resolved
cypress/support/commands.ts Outdated Show resolved Hide resolved
cypress/support/e2e.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,14 @@
export default class CustomComponent extends HTMLElement {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기회가 되면.. 더 적용해보겠습니다!! 👍

src/components/AppComponent.js Show resolved Hide resolved
src/components/AppHeaderComponent.js Outdated Show resolved Hide resolved
src/components/AppHeaderComponent.js Outdated Show resolved Hide resolved
Comment on lines +2 to +5
beforeEach(() => {
cy.visit("http://localhost:8081/");
cy.viewport(1920, 1080);
});
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

viewport 말씀하시는건가요?? intercept를 접속 이전에 실행해야 되는걸로 알고 있어 나머지는 중복으로 작성했는데, 다시 한번 알아볼께요!

cypress/e2e/movie-e2e.cy.ts Outdated Show resolved Hide resolved
src/components/movie/MovieComponent.js Outdated Show resolved Hide resolved
movieItems: MovieItem[]
): MovieElementData[] => {
return movieItems.map(
({ poster_path, id, title, vote_average }: MovieItem) => ({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
({ poster_path, id, title, vote_average }: MovieItem) => ({
({ poster_path, id, title, vote_average }) => ({

이렇게하더라도 충분히 타입추론은 되는것 같군요 :)

@devhyun637 devhyun637 merged commit 3f9c0b0 into woowacourse:2yunseong Mar 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants